-
Notifications
You must be signed in to change notification settings - Fork 350
[babel-plugin][legacy] Polyfill logical float values #1237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
mellyeliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for taking this on! Some follow ups :)
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/legacy-expand-shorthands.js
Outdated
Show resolved
Hide resolved
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/legacy-expand-shorthands.js
Outdated
Show resolved
Hide resolved
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/index.js
Outdated
Show resolved
Hide resolved
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/index.js
Outdated
Show resolved
Hide resolved
| { | ||
| "ltr": ".xodj72a{clear:right}", | ||
| "rtl": ".xodj72a{clear:left}", | ||
| "rtl": ".xodj72a{clear:var(--1t497je)}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we should be seeing this as the output:
"ltr": ".xodj72a{clear:var(--1t497je)}"",
"rtl": null,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮 I had that earlier but thought it was wrong
might have to change some of the logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing the float mappings in generateRtl should fix it :)
the context is with this change we can get rid of the rtl field completely! we'll have one value that autoflips
|
|
||
| // Export both the expansions object and the processing function | ||
| export default expansions as typeof expansions; | ||
| export { processProperty }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I will remove it
| }> = { | ||
| float: ([key, val]) => [key, logicalToPhysical[val] ?? val], | ||
| clear: ([key, val]) => [key, logicalToPhysical[val] ?? val], | ||
| start: `var(${LOGICAL_FLOAT_END_VAR})`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking closer, the float mapping in legacy-expand-shorthands should be enough, we don't need to add handling in generate-rtl.js or generate-rtl.js, we just need to remove the blocks listed in #1217
| start: `var(${LOGICAL_FLOAT_END_VAR})`, | ||
| end: `var(${LOGICAL_FLOAT_START_VAR})`, | ||
| 'inline-start': `var(${LOGICAL_FLOAT_END_VAR})`, | ||
| 'inline-end': `var(${LOGICAL_FLOAT_START_VAR})`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| start: `var(${LOGICAL_FLOAT_END_VAR})`, | |
| end: `var(${LOGICAL_FLOAT_START_VAR})`, | |
| 'inline-start': `var(${LOGICAL_FLOAT_END_VAR})`, | |
| 'inline-end': `var(${LOGICAL_FLOAT_START_VAR})`, |
| if (value === 'inline-start') { | ||
| return [[property, `var(${LOGICAL_FLOAT_START_VAR})`]]; | ||
| } else if (value === 'inline-end') { | ||
| return [[property, `var(${LOGICAL_FLOAT_END_VAR})`]]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (value === 'inline-start') { | |
| return [[property, `var(${LOGICAL_FLOAT_START_VAR})`]]; | |
| } else if (value === 'inline-end') { | |
| return [[property, `var(${LOGICAL_FLOAT_END_VAR})`]]; | |
| } | |
| if (value === 'inline-start' || value === 'start') { | |
| return [[property, `var(${LOGICAL_FLOAT_START_VAR})`]]; | |
| } else if (value === 'inline-end' || value === 'end') { | |
| return [[property, `var(${LOGICAL_FLOAT_END_VAR})`]]; | |
| } |
|
Almost, just a couple more fixes! |
mellyeliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! Let's hold off on merging this for now because this will involve a prerequisite change on the Haste side
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/legacy-expand-shorthands.js
Outdated
Show resolved
Hide resolved
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/legacy-expand-shorthands.js
Outdated
Show resolved
Hide resolved
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/legacy-expand-shorthands.js
Outdated
Show resolved
Hide resolved
22973bd to
c95f91f
Compare
c95f91f to
2dacb1e
Compare
workflow: benchmarks/perfComparison of performance test results, measured in operations per second. Larger is better.
|
workflow: benchmarks/sizeComparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.
|
| { | ||
| "ltr": ".xodj72a{clear:right}", | ||
| "rtl": ".xodj72a{clear:left}", | ||
| "ltr": ".xodj72a{clear:end}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output needs to be: .xodj72a{clear: var(--stylex-logical-end)}
|
|
||
| import splitValue from '../utils/split-css-value'; | ||
|
|
||
| export const LOGICAL_FLOAT_START_VAR = '--stylex-logical-end'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add logic here to map float: start and float: inline-start to float: var(LOGICAL_FLOAT_START_VAR)
2dacb1e to
1bfaeb7
Compare
mellyeliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just missing the processStylexRules change that seem to be lost in the merge. Will stamp after that!
packages/@stylexjs/babel-plugin/src/shared/preprocess-rules/legacy-expand-shorthands.js
Outdated
Show resolved
Hide resolved
…es, going to use it
mellyeliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff. Thanks for sitting through all the iterations! Please add back the cleanup in generate-ltr and generate-lrtl before merging
* addressed Melissa' suggestions * More changes that are hopefully correct * Version that doesn't check for float and clear * The first version is probably correct as it passes all the other suites, going to use it * last bit of changes missed + updating tests --------- Co-authored-by: Anay Bhakat <[email protected]>
What changed / motivation ?
context
Linked PR/Issues
Fixes #1217
Additional Context
npm test passes
updated the snapshots for anything my changes impact
Pre-flight checklist
Contribution Guidelines